Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[node-core-library] Add support for alreadyExistsBehavior for symlinks and junctions #2884

Merged
merged 10 commits into from
Sep 11, 2021

Conversation

D4N14L
Copy link
Member

@D4N14L D4N14L commented Sep 1, 2021

Summary

Add support for alreadyExistsBehavior for symlinks and junctions to align with hardlinks. Additionally, fix various error scenarios that can be encountered for all types of links.

Details

Support for alreadyExistsBehavior was added for hardlinks in #2333, however it was not wired up for symlinks and junctions even though the options interface was used by these methods as well.

Additionally, while adding support for these types of links, it was realized that there were a few error scenarios that were not being handled correctly:

  • AlreadyExistsBehavior.overwrite would delete the existing file but not create the new link
  • AlreadyExistsBehavior.overwrite would fail if the existing entity is a directory instead of a file
  • linkTargetPath may or may not exist depending on the type of link being created, causing symlinks and junctions to non-existent files to fail (even though they are valid)

How it was tested

Tested each link type in a local sandbox to probe error-handling scenarios, namely:

  • Create link (all types) to non-existent file/folder
  • Create link (all types) to existing file/folder in directory that doesn't exist
  • Create link (all types) to existing file/folder in directory with conflict
  • Create link (all types) to existing file/folder in directory with conflict and AlreadyExistsBehavior.overwrite

@D4N14L D4N14L changed the title Add support for alreadyExistsBehavior for symlinks and junctions [node-core-library] Add support for alreadyExistsBehavior for symlinks and junctions Sep 1, 2021
@D4N14L
Copy link
Member Author

D4N14L commented Sep 7, 2021

@octogonz - let me know once you get a chance to test this so that we can get this merged, since it seems like a big hole in NCL.

@octogonz
Copy link
Collaborator

let me know once you get a chance to test this so that we can get this merged, since it seems like a big hole in NCL.

I should have some time tomorrow.

@octogonz
Copy link
Collaborator

Looking at this, I'm increasingly uncomfortable with the idea that AlreadyExistsBehavior.Overwrite can recursively delete folders. It's like if you did ln -sf a b and the effect was rm -Rf b. Unix ln does not overwrite folders.

@octogonz octogonz enabled auto-merge September 11, 2021 04:47
@octogonz octogonz merged commit b2aa54f into microsoft:master Sep 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants